Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IF-STRIDE-STAKEIBC-UPDATEDELEGATIONBALANCES fix #447

Merged
merged 5 commits into from
Dec 3, 2022

Conversation

asalzmann
Copy link
Contributor

@asalzmann asalzmann commented Dec 3, 2022

Underflow possible when decreasing staked balance for host zone

Involved artifacts

Description

When undelegating from each validator, host zone's staked balance is being decreased according to undelegation amount of particular validator:

for _, undelegation := range undelegateCallback.SplitDelegations {
		undelegateAmt, err := cast.ToInt64E(undelegation.Amount)
		k.Logger(ctx).Info(fmt.Sprintf("UndelegateCallback, Undelegation: %d, validator: %s", undelegateAmt, undelegation.Validator))
		if err != nil {
			errMsg := fmt.Sprintf("Could not convert undelegate amount to int64 in undelegation callback | %s", err.Error())
			k.Logger(ctx).Error(errMsg)
			return sdkerrors.Wrapf(types.ErrIntCast, errMsg)
		}
		undelegateVal := undelegation.Validator
		success := k.AddDelegationToValidator(ctx, zone, undelegateVal, -undelegateAmt)
		if !success {
			return sdkerrors.Wrapf(types.ErrValidatorDelegationChg, "Failed to remove delegation to validator")
		}
		zone.StakedBal -= undelegation.Amount
	}

However, zone.StakedBal is defined as uint64; therefore, it might underflow if undelegation.Amount > zone.StakedBal. Undelegation amount for each undelegation is calculated in GetTargetValAmtsForHostZone and have been additionaly modified during overflow handling.

Problem Scenarios

If underflow happens, staked balance of particular zone will unexpectedly and quietly become tremendously large having adverse consequences during both, UndelegateCallback and DelegateCallback.

Recommendation

Add validation check before decreasing the staked balance amount:

  if undelegation.Amount > zone.StakedBal {
  // handle incoming underflow
  } else {
    zone.StakedBal -= undelegation.Amount
  }

@asalzmann asalzmann requested a review from a team December 3, 2022 18:34
@asalzmann asalzmann marked this pull request as ready for review December 3, 2022 18:34
Copy link
Collaborator

@sampocs sampocs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@asalzmann asalzmann added the v4 label Dec 3, 2022
@asalzmann asalzmann added the A:automerge Automatically merge PR once checks pass label Dec 3, 2022
@mergify mergify bot merged commit a8a1658 into main Dec 3, 2022
sontrinh16 pushed a commit to notional-labs/stride that referenced this pull request Mar 27, 2023
Underflow possible when decreasing staked balance for host zone

# Involved artifacts

- [x/stakeibc/keeper/icacallbacks_undelegate.go](https://github.com/Stride-Labs/stride/blob/v2.0.3/x/stakeibc/keeper/icacallbacks_undelegate.go#L128)

# Description

When undelegating from each validator, host zone's staked balance is being decreased according to undelegation amount of particular validator:

```go
for _, undelegation := range undelegateCallback.SplitDelegations {
		undelegateAmt, err := cast.ToInt64E(undelegation.Amount)
		k.Logger(ctx).Info(fmt.Sprintf("UndelegateCallback, Undelegation: %d, validator: %s", undelegateAmt, undelegation.Validator))
		if err != nil {
			errMsg := fmt.Sprintf("Could not convert undelegate amount to int64 in undelegation callback | %s", err.Error())
			k.Logger(ctx).Error(errMsg)
			return sdkerrors.Wrapf(types.ErrIntCast, errMsg)
		}
		undelegateVal := undelegation.Validator
		success := k.AddDelegationToValidator(ctx, zone, undelegateVal, -undelegateAmt)
		if !success {
			return sdkerrors.Wrapf(types.ErrValidatorDelegationChg, "Failed to remove delegation to validator")
		}
		zone.StakedBal -= undelegation.Amount
	}
```

However, `zone.StakedBal` is defined as `uint64`; therefore, it might underflow if `undelegation.Amount` > `zone.StakedBal`. Undelegation amount for each undelegation is calculated in [GetTargetValAmtsForHostZone](https://github.com/Stride-Labs/stride/blob/v2.0.3/x/stakeibc/keeper/unbonding_records.go#L74) and have been additionaly modified during [overflow](https://github.com/Stride-Labs/stride/blob/v2.0.3/x/stakeibc/keeper/unbonding_records.go#L103-L139) handling.


# Problem Scenarios

If underflow happens, staked balance of particular zone will unexpectedly and quietly become tremendously large having adverse consequences during both, `UndelegateCallback` and `DelegateCallback`.


# Recommendation

Add validation check before decreasing the staked balance amount: 

```go
  if undelegation.Amount > zone.StakedBal {
  // handle incoming underflow
  } else {
    zone.StakedBal -= undelegation.Amount
  }
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once checks pass C:stakeibc v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants